feat(agent): forward model settings and recover from thinking stalls [PC-4672] - #1017
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves agent/tool-call reliability under Anthropic “thinking” modes across transports, adds safer provider HTTP error normalization, and introduces model_settings forwarding for the new chat model factory path.
Changes:
- Add shared “thinking” utilities and Bedrock handler logic to downgrade forced tool-choice when thinking is active, plus a forced-extraction retry path in the ReAct LLM node.
- Refactor ReAct routing/stall accounting from router-level “thinking message limits” to LLM-node stall handling, with new/updated tests.
- Add
model_settingsparameter toget_chat_model, warn when ignored on legacy clients, and bumpuipath-langchain-clientdependency.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/uipath_langchain/chat/thinking.py |
Introduces shared helpers for detecting/stripping thinking and reasoning blocks. |
src/uipath_langchain/chat/handlers/bedrock.py |
Uses shared predicate to downgrade forced tool choice under thinking on Bedrock. |
src/uipath_langchain/chat/chat_model_factory.py |
Adds model_settings arg, forwards to new factory, warns on legacy path. |
src/uipath_langchain/agent/react/llm_node.py |
Moves stall handling into LLM node; adds forced-extraction retry and deterministic failure. |
src/uipath_langchain/agent/react/router.py |
Removes thinking-limit enforcement; routes tool-less content back to agent. |
src/uipath_langchain/agent/react/utils.py |
Renames & documents stall counter as “tool-less turns”. |
src/uipath_langchain/agent/react/forced_extraction.py |
New helper to strip thinking + reasoning blocks and append trailing user turn. |
src/uipath_langchain/agent/exceptions/llm.py |
Consolidates provider HTTP error mapping and adds category/status handling. |
src/uipath_langchain/agent/exceptions/licensing.py |
Removes legacy provider HTTP error mapping module. |
tests/... |
Adds/updates tests for model_settings dispatch, Bedrock downgrade behavior, forced extraction, routing changes, and HTTP error redaction. |
pyproject.toml |
Version bump and dependency range bump for uipath-langchain-client. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
29cce8a to
9b88d18
Compare
| # Maps known LLM Gateway status codes to specific error codes. | ||
| # Unknown status codes fall back to HTTP_ERROR. | ||
| _LLM_STATUS_CODE_MAP: dict[int, AgentRuntimeErrorCode] = { | ||
| 403: AgentRuntimeErrorCode.LICENSE_NOT_AVAILABLE, |
There was a problem hiding this comment.
this mapping is incorrect. I know you just moved the mapping, but this should be fixed.
| raise AgentRuntimeError( | ||
| code=code, | ||
| title=f"LLM provider returned HTTP {status_code}", | ||
| detail=detail or _GENERIC_HTTP_DETAIL, |
There was a problem hiding this comment.
add error.message like before
The generic detail and `from None` were guarding against UiPathAPIError.__str__ embedding the raw response body. That is being fixed at the source in uipath-llm-client, so both guards are redundant here and the original mapping (gateway detail, falling back to the reason phrase) is restored. Keeps only the licensing.py -> llm.py module merge; the file name no longer described what the module does now that it maps all provider errors.
The `model_settings` parameter this forwards through `get_chat_model` only exists in uipath-langchain-client 1.18.3, so the floor moves on all seven extras. The `<1.19.0` ceiling is unchanged.
afc7348 to
56e938e
Compare
CI runs mypy over the whole repo, including tests. The stand-in chat models were assigned over typed methods and passed to a BaseChatModel parameter, so the checks failed there while `mypy src` stayed green.
56e938e to
6828913
Compare
|



Anthropic won't honor a forced
tool_choicewhile extended or adaptive thinking is on, so a thinking model can answer in plain text and never callend_execution. The old consecutive-thinking counter turned that into a hardTHINKING_LIMIT_EXCEEDEDon the first occurrence. Now one tool-less turn is tolerated and retried with thinking off and the tool call forced (react/forced_extraction.py), which every provider honors; only a second stall fails, with the same error code.Stall accounting moved out of the router and into the LLM node, because the router can't tell whether forcing actually survived on the wire: the Bedrock handlers silently downgrade
anytoautounder thinking. A tool-less turn with content now always loops back to the agent.AgentGraphConfig.thinking_messages_limitis no longer read anywhere; I left the field in place so existingagent.jsonfiles don't break, but it's dead, and I'd rather deprecate it properly in a follow-up than widen this PR.New
chat/thinking.pyholds the transport-specific knowledge in one place (nativethinkingattribute,model_kwargsfor Bedrock Invoke,additional_model_request_fieldsfor Converse) so the payload handlers and the ReAct loop stop each reimplementing it. Replaying reasoning blocks on a thinking-off call 400s, so the extraction retry strips them and appends a user turn if the history ends on an AI message. That trailing user turn is load-bearing rather than cosmetic: without it the retry ends on an assistant message, which is a prefill, and prefill 400s on Claude 4.6 and later.Two smaller things ride along.
get_chat_modelgains amodel_settingspassthrough for the client-side work in uipath-langchain-client, with a warning when the legacy clients would silently drop it, andexceptions/licensing.pyfolds intoexceptions/llm.pyso both LLM error shapes are mapped from one module. The fold is a pure move:raise_for_provider_http_errorbehaves exactly as it did on main. An earlier revision of this branch also made it withhold the vendor message from the run record, which I reverted, so provider error text still reaches telemetry the way it does today. That's worth fixing, but at the telemetry boundary rather than at every raise site, and not here.Start with
react/llm_node.py; everything else follows from the escalation ladder it implements.Version bumped to 0.16.15, and the
uipath-langchain-clientfloor moves to 1.18.3 on all seven extras since that's where themodel_settingsparameter lands.Verified with the 185 tests across the touched files, plus the full suite, mypy over the repo, and the integration matrices in CI.